Skip to content

Support type stub generation for staticmethod#14934

Merged
hauntsaninja merged 13 commits into
python:masterfrom
WeilerMarcel:support_staticmethod
Jan 8, 2024
Merged

Support type stub generation for staticmethod#14934
hauntsaninja merged 13 commits into
python:masterfrom
WeilerMarcel:support_staticmethod

Conversation

@WeilerMarcel

Copy link
Copy Markdown
Contributor

Fixes #13574

This PR fixes the generation of type hints for static methods of pybind11 classes. The code changes are based on the suggestions in #13574.

The fix introduces an additional check if the property under inspection is of type staticmethod. If it is, the type information is read from the staticmethod's __func__ attribute, instead of the staticmethod instance itself.

I added a test for C++ classes with static methods bound using pybind11. Both, an overloaded and a non-overloaded static method are tested.

Comment thread mypy/stubgenc.py Outdated
@WeilerMarcel

Copy link
Copy Markdown
Contributor Author

Hi @sobolevn and @AlexWaygood, I updated this PR to work with the latest changes on master. Could you please review the current changes and merge the PR if everything is fine?

@WeilerMarcel

Copy link
Copy Markdown
Contributor Author

Hey @sobolevn and @AlexWaygood, what is the status of this pull request? I would really like to see it merged into MyPy. Do you need anything from my side to proceed?

@d4n1elchen

Copy link
Copy Markdown

Looking forward to this getting merged as well +1

@WeilerMarcel

Copy link
Copy Markdown
Contributor Author

Hi @sobolevn and @AlexWaygood, please give me an update on this PR. Is there a reason why it has not been merged? As far as I can see, I can not merge it myself since I don't have the necessary permissions.

@sobolevn

Copy link
Copy Markdown
Member

I can't merge this PR, because I don't fully understand the C++ code, but I think that @JukkaL or @msullivan can help us :)

@AlexWaygood

Copy link
Copy Markdown
Member

I can't merge this PR because I'm not a maintainer

@WeilerMarcel

Copy link
Copy Markdown
Contributor Author

Thanks for the info :) I was under the impression that you could merge, since you were reviewing my changes.
I will wait for a reply from @JukkaL and @msullivan.

@sobolevn

Copy link
Copy Markdown
Member

In the meantime you can resolve conflicts :)

@WeilerMarcel

Copy link
Copy Markdown
Contributor Author

Hi everyone, I integrated my changes into the new master branch and from my perspective it is ready for merging.

I noticed, though, that the mypyc runtime tests with py39-macos seem to be unstable. Since I did not find a way to rerun tests for a commit, I added two empty dummy commits to get the pipeline to succeed. See #16420.

Comment thread mypy/stubgenc.py Outdated
@JukkaL

JukkaL commented Nov 8, 2023

Copy link
Copy Markdown
Collaborator

You can ignore the mypyc runtime test failures, since it's a known issue happening on master.

Comment thread mypy/stubgenc.py Outdated
if class_info is None:
return False
elif self.is_c_module:
raw_lookup = getattr(class_info.cls, "__dict__") # noqa: B009

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has this been # noqa'd? Wouldn't it be cleaner to do this, as the linter suggests?

Suggested change
raw_lookup = getattr(class_info.cls, "__dict__") # noqa: B009
raw_lookup = class_info.cls.__dict__

@WeilerMarcel WeilerMarcel Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I wrote it that way because there are two other places in stubgenc.py where this pattern was used:

  • mypy/mypy/stubgenc.py

    Lines 453 to 455 in 8121e3c

    def get_members(self, obj: object) -> list[tuple[str, Any]]:
    obj_dict: Mapping[str, Any] = getattr(obj, "__dict__") # noqa: B009
    results = []
  • mypy/mypy/stubgenc.py

    Lines 718 to 725 in 8121e3c

    def generate_class_stub(self, class_name: str, cls: type, output: list[str]) -> None:
    """Generate stub for a single class using runtime introspection.
    The result lines will be appended to 'output'. If necessary, any
    required names will be added to 'imports'.
    """
    raw_lookup = getattr(cls, "__dict__") # noqa: B009
    items = self.get_members(cls)

    I will check the file history to see if I can find a specific reason why getattr is used.

@WeilerMarcel WeilerMarcel Nov 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getattr call originally comes from commit 6bcaf40 and was added to make typeshed happy:

mypy/mypy/stubgenc.py

Lines 140 to 142 in 6bcaf40

# typeshed gives obj.__dict__ the not quite correct type Dict[str, Any]
# (it could be a mappingproxy!), which makes mypyc mad, so obfuscate it.
obj_dict = getattr(obj, '__dict__') # type: Mapping[str, Any]

The # noqa was added in 4287af4:

obj_dict = getattr(obj, '__dict__') # type: Mapping[str, Any] # noqa

@WeilerMarcel

Copy link
Copy Markdown
Contributor Author

Hi @JukkaL and @msullivan, I resolved conflicts with the current master branch. Can this PR be merged?

@WeilerMarcel

Copy link
Copy Markdown
Contributor Author

Happy new year everyone! 🥳
Are there plans to merge this PR yet? I would really like to get this off my mind.

@JukkaL

JukkaL commented Jan 3, 2024

Copy link
Copy Markdown
Collaborator

I can merge this PR if somebody who has more context on the relevant code than I have gives a quick thumbs up.

@JukkaL

JukkaL commented Jan 3, 2024

Copy link
Copy Markdown
Collaborator

more context on the relevant code than I have

Or somebody who has any familiarity with the code :-)

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that familiar with stubgen, but this mostly looks good to me. I had one question

Comment thread mypy/stubgenc.py Outdated

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! :-)

@hauntsaninja hauntsaninja merged commit 35f402c into python:master Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stubgenc.py does not generate correct lines for staticmethods

6 participants